-
Notifications
You must be signed in to change notification settings - Fork 305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: 非同期処理を改良 #1973
perf: 非同期処理を改良 #1973
Conversation
ソングのみのキャラクター対応
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ほぼLGTMです!!
元のコード(僕が書きました。。)よりずっとわかりやすいと感じました!!
src/store/audio.ts
Outdated
const characterInfosPromise = diffArrays( | ||
speakers.map((speaker) => speaker.speakerUuid), | ||
singers.map((singer) => singer.speakerUuid) | ||
).flatMap((diff) => { | ||
const isSpeaker = diff.removed || !diff.added; | ||
const isSinger = diff.added || !diff.removed; | ||
return diff.value.map((speakerUuid) => { | ||
const speaker = isSpeaker | ||
? speakers.find((speaker) => speaker.speakerUuid === speakerUuid) | ||
: undefined; | ||
const singer = isSinger | ||
? singers.find((singer) => singer.speakerUuid === speakerUuid) | ||
: undefined; | ||
return getCharacterInfo(speaker, singer); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diffArray使うのも良さそうですが、diffライブラリに依存しちゃうのはなるべく避けておきたいかもです。
(爆速なら話は別ですが、まあデータ数も少なそうですし)
Setを使うのはどうでしょう。こんな感じ?
const speakerUuids = new Set(speakers.map(speaker => speaker.speakerUuid));
const singerUuids = new Set(singers.map(singer => singer.speakerUuid));
const allUuids = new Set([...speakerUuids, ...singerUuids]);
const characterInfosPromise = Array.from(allUuids).map(speakerUuid => {
const isSpeaker = speakerUuids.has(speakerUuid);
const isSinger = singerUuids.has(speakerUuid);
const speaker = isSpeaker ? speakers.find(speaker => speaker.speakerUuid === speakerUuid) : undefined;
const singer = isSinger ? singers.find(singer => singer.speakerUuid === speakerUuid) : undefined;
return getCharacterInfo(speaker, singer);
});
getCharacterInfoがundefined渡せる前提なら、もっと簡略化して、こう・・・?
const allUuids = new Set([...speakers, ...singers].map(item => item.speakerUuid));
const characterInfosPromise = Array.from(allUuids).map(speakerUuid => {
const speaker = speakers.find(speaker => speaker.speakerUuid === speakerUuid);
const singer = singers.find(singer => singer.speakerUuid === speakerUuid);
return getCharacterInfo(speaker, singer);
});
あ、これcharacterInfosPromise
じゃなくてcharacterInfoPromises
かもですね
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setを使うとトークのみ、ソングのみのキャラクターの順序の維持ができなくなりますね…
const speakers = ["a", "b", "c", "e"];
const singers = [ "a", "c", "d", "e"];
const characters = Array.from(new Set([...speakers, ...singers]));
console.log(characters); // [ 'a', 'b', 'c', 'e', 'd' ] "c"と"e"の間にあるべき"d"が一番後ろになる
ソングのみのキャラクターが一番後ろになっても構わないのならSet
を使うのがいいと思います。
順序を維持してマージをする処理を自分で書くのは自分では無理だと判断したのとdiff
が別のところで使われていることを思い出したので使用しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あーーーーーーーーなるほどです!!! 気づいてくださってありがとうございます 🙇 🙇 🙇
将来忘れそうなので、コメントで「順番を維持しつつキャラクター情報を取得」とか書くのお願いできると。。 🙇
今気づきましたが、diffだとsingersが例えばe, a
だったときバグりそうですね・・・。
ちょっと効率の悪いコードな気がしますが、speakersに未知singers足す形が手っ取り早い気がしました。
こんな感じ?
// 順番を維持しつつキャラクター情報を取得
const speakerUuids = speakers.map((speaker) => speaker.speakerUuid)
const singerUuids = singers.map((singer) => singer.singerUuid)
// 配列を足すtsコードこれで合ってるのか自信ない。。
[...speakerUuids, ...singerUuids.filter(uuid=>!speakerUuids.has(uuid))].map(uuid => {
// promiseを返すコード
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このコードだと結局Set
を使ったコードと同じになってしまう気が…
今気づきましたが、diffだとsingersが例えばe, aだったときバグりそうですね・・・。
const speakers = ["a", "b", "d"]
const singers = ["c", "a", "b", "d"]
なら ["c", "a", "b", "d"]
になるはずです。
ただ、両方に存在するが位置関係が一致しない場合はバグになりますね…
const speakers = ["a", "b", "c", "e"];
const singers = [ "e", "a", "c", "d"];
// 多分 [ "e", "a", "b", "c", "e", "d"] になる
現在のVOICEVOXエンジンの場合は問題ありませんが今後のことやサードパーティ製エンジンのことも考えるとSet
しかないですね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CoreでSpeakerに順番の情報が入ったので、やるならこれをEngineでも吐くようにしてそれをベースに並び替えとかですかね?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sevenc-nanashi
たしかに、だいぶ先の話になりそうですが順番入れても良いかもですね!!
というか入れとかないとこうなる。
このコードだと結局Setを使ったコードと同じになってしまう気が…
あーーーーなるほどです!!!
歌手のみが最後になるの、なるほどです。問題を何かと勘違いしていました。
ちょっとどうしようもなさそうですね…!!
一旦今はそういう問題が起こりうる、という認識で進めるしかなさそう。
ちょっと一言fixmeコメントと、この議論へのリンクをコメントお願いできると🙇
そもそも全キャラ情報を一つの配列で持ってるのが問題の根本な気がしました。
話者と歌手別々に配列持っといて、複合したのが必要なら毎回合成すれば良さそう。(現状たぶん合成がいる箇所は無い)
…というissueをPRマージ時に立てると良さそう!忘れそうだけど!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ、ちなみにsetはたぶん順番が変わりうるので微妙かもです!
順番変わらないというドキュメントがあればその前提で使ってもよさそう。(たしかMapにはそういうドキュメントが書かれてある)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/Set
挿入順は、各要素が add メソッドによって正常に Set に挿入された順番に対応します。
順番は保存されそう?(知らなかった)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set
で順番は変わらないみたいなのでSet
を使いました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
手元で測った感じ、もしかしたら実行時間が10%未満ほど伸びてるかもでした。
まあエンジン側に同時リクエストが走るので伸びていても不思議ではないかもですが、そもそもアーキテクチャーをなんとかするべきなのと、コードは見やすくなっているのとで、マージするのがいいのかなと思いました!
ちょっとこちらで一部変更させていただいてからマージしたいと思います!
内容
CharacterInfo
の作成をより並行に実行するように変更します。また、ソングのみのキャラクター対応も行います。
その他
ソングのみのキャラクター対応はdiffArray
を使用して一方しかなくても順序が維持されるようにしました。しかし、一方のみのキャラクターが交互に続いた場合の対応は不可能です。/speakers
と/singers
や/speaker_info
と/singer_info
を同時にリクエストするようにしたり、リクエスト処理の間にStyleInfo
の作成を進めたりするようにしました。(base64のデコードがやや重い処理のようです)ただ可読性が下がってしまい自分にはこれ以上なんとかできませんでした。